-
-
Notifications
You must be signed in to change notification settings - Fork 260
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove old code that existed for now unsupported ember-sources (2.4, etc) #1486
Conversation
c8db6d8
to
d26589e
Compare
this.setProperties({ | ||
jax: helper(([name]) => `${name}-jax`), | ||
}); | ||
await render(hbs`{{this.jax "max"}}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these helpers were (rightfully so!) trying to look up the helper defined in app/helpers/jax.
However, that doesn't exist, and since we don't actually support custom resolvers for normal apps, this is fine. These tests are testing that render
can invoke helpers, and helpers can be defined on this
addon/src/settled.ts
Outdated
if ( | ||
EmberTest.waiters.some(([context, callback]) => !callback.call(context)) | ||
) { | ||
return true; | ||
} | ||
} | ||
|
||
return false; | ||
return EmberTest.checkWaiters(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changing this from false
to EmberTest.checkWaiters()
is a meaningful change, but the only mainingful change in this PR (for consumers).
The code for checkWaiters
is here: https://github.com/emberjs/ember.js/blob/85a4f298f67ff70395cf7f9103682335162e0606/packages/ember-testing/lib/test/waiters.ts#L111
and those callbacks are part of the legacy waiter system.
There are existing comments about removing code for accessing legacy-waiter stuff (because ember.js has since abstracted it so we don't need to be so weird in this library) -- cleaning that up can happen in a followup PR, and should be non-breaking, and retain legacy waiter support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So.... I suppose this should be a patch-release, rather than non-release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me it looks fine, but I'm not familiair with the in-depths aspects of it all.
… the problem with embroider optimized
We just run into an issue where tests were failing when switching to Embroider optimized (works for safe mode!) due to |
Ye! I'm hopeful -- we need this tho: This test helpers pr currently contains a patch |
aedaf96
to
5b7e9f7
Compare
Updated embroider (via lockfile bumps), and things are green now without the patch! |
|
||
import EmberApplicationInstance from '@ember/application/instance'; | ||
import { Test } from 'ember-testing'; | ||
export const checkWaiters = Test.checkWaiters; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this being exported? This seems to have caused a type regression, see #1502
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great question -- I had originally thought that the original implementation was exported -- but I see in this diff that it is not. let's see if not exporting it resolves that issue
NOTE:
Not having embroider-optimized enabled has caused some bug reports related to resolving packages (
@ember/-internals
,@ember/renderer
).See: #1487
embroider-optimized testing was disabled in 6828d0c#diff-efa7561fd196a5304c1c80012f1f6153c58faf5697ba3a59465ae7ce06b8e550R146
(in August 2021)
This library's minimum supported ember version is 4.0: https://github.com/emberjs/ember-test-helpers